arrow-buffer: i256: implement ilog by theirix · Pull Request #9453 · apache/arrow-rs

theirix · 2026-02-20T21:14:02Z

Which issue does this PR close?

Closes arrow-buffer: implement log for i256 #9452.

Rationale for this change

Implementation of integer logarithm. There is no matching num_traits trait, but this implementation provides a good motivation for such a trait.

What changes are included in this PR?

No external dependencies
Checked methods (log, log2, log10)
Unchecked methods (panic by design)

Are these changes tested?

Unit tests

Are there any user-facing changes?

No

Rafferty97 · 2026-03-12T22:54:20Z

+    /// Returns `None` if `self` is less than or equal to zero, or if `base` is less than 2.
+    #[inline]
+    pub fn checked_ilog(self, base: i256) -> Option<u32> {
+        if self <= Self::ZERO || base < i256::from(2) {


Since these invariants are checked by the call to ExtI256::checked_ilog anyway, what's the point of restating the logic here? The to_ext_i256 calls should be no-ops anyway.

The reason was to place checks closer to the caller. For procmacros of i256, it's less clear from the source code, but the documentation explicitly lists the required checks, and we repeat them in a docstring. Removed extra code.

Rafferty97 · 2026-03-12T23:08:54Z

+    /// Returns `None` if `self` is less than or equal to zero.
+    #[inline]
+    pub fn checked_ilog10(self) -> Option<u32> {
+        // Switch to I256::checked_ilog10 when I256 switches MSRV


Why can't it be used now?

checked_ilog10 is literally commented out in i256 code: Alexhuszagh/i256-rs@85c0bb1/src/int/checked.rs#L102

Ah, I only checked checked_ilog2. My mistake :)

Rafferty97 · 2026-03-12T23:09:03Z

+    /// Returns `None` if `self` is less than or equal to zero.
+    #[inline]
+    pub fn checked_ilog2(self) -> Option<u32> {
+        // Switch to I256::checked_ilog2 when I256 switches MSRV


Same as above: why not now?

For checked_ilog2 - it exists, updated the call.

Rafferty97 · 2026-03-12T23:11:07Z

+        // log10 should be 76 or 77
+        let result = large.checked_ilog(i256::from(10));
+        assert!(result.is_some());
+        assert!(result.unwrap() >= 76 && result.unwrap() <= 77);


Why can't an assert_eq! be used here? Surely the result is deterministic?

alamb · 2026-04-22T17:49:07Z

I am not sure about adding a new dependency on the i256 crate. It seems that it is not widely used across the ecosystem

https://crates.io/crates/i256
https://crates.io/crates/i256/reverse_dependencies

This is the kind of crate that I worry will end up having a RUSTSEC against it (due to unmaintained, etc) and I think will add an additional maintenance burden

theirix · 2026-04-22T18:55:24Z

I am not sure about adding a new dependency on the i256 crate. It seems that it is not widely used across the ecosystem

crates.io/crates/i256 crates.io/crates/i256/reverse_dependencies

This is the kind of crate that I worry will end up having a RUSTSEC against it (due to unmaintained, etc) and I think will add an additional maintenance burden

That's true - it is much less popular than other math libraries. I could suggest that the bnum crate is much more widely used and provides the required primitives. If we prefer to avoid a dependency on a smaller crate, it's pretty simple to implement arithmetic manually, as is already done with checked_pow.

alamb · 2026-04-24T16:26:25Z

That's true - it is much less popular than other math libraries. I could suggest that the bnum crate is much more widely used and provides the required primitives. If we prefer to avoid a dependency on a smaller crate, it's pretty simple to implement arithmetic manually, as is already done with checked_pow.

This would be my preference -- "a little copying vs a little of dependency"

https://www.youtube.com/clip/UgkxWCEmMJFW0-TvSMzcMEAHZcpt2FsVXP65

theirix · 2026-04-24T17:35:48Z

That's true - it is much less popular than other math libraries. I could suggest that the bnum crate is much more widely used and provides the required primitives. If we prefer to avoid a dependency on a smaller crate, it's pretty simple to implement arithmetic manually, as is already done with checked_pow.

This would be my preference -- "a little copying vs a little of dependency"

youtube.com/clip/UgkxWCEmMJFW0-TvSMzcMEAHZcpt2FsVXP65

Thank you, that's insightful! I'll update the implementation shortly.

theirix · 2026-05-24T08:36:00Z

That's true - it is much less popular than other math libraries. I could suggest that the bnum crate is much more widely used and provides the required primitives. If we prefer to avoid a dependency on a smaller crate, it's pretty simple to implement arithmetic manually, as is already done with checked_pow.

This would be my preference -- "a little copying vs a little of dependency"
youtube.com/clip/UgkxWCEmMJFW0-TvSMzcMEAHZcpt2FsVXP65

Thank you, that's insightful! I'll update the implementation shortly.

@alamb I've switched to the native i256 support without external deps. Could you please re-review?

Extends the existing `test_ilog` with cases that the current tests do not cover: small results (0 and 1) for non-base-10 bases, including `self == base`. Per the std contract `n.ilog(n) == 1` (e.g. `2u32.ilog(2) == 1`), but `i256::checked_ilog` currently returns 0 for these inputs. Also cross-checks small results against `u128::ilog` as ground truth. These tests fail until the following fix is applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`checked_ilog` returned the early `Some(0)` for `self == base`, but the correct result is 1 (`base^1 == base <= self`), matching the std contract `n.ilog(n) == 1`. The base-10 path was unaffected since it is handled by `checked_ilog10`, so this affected every other base (including `ilog2(2)`). Narrow the early return from `self <= base` to `self < base`; the loop already computes the correct value for `self == base`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alamb

Thanks @theirix and @Rafferty97 -- this is very close, but i think I found a bug (ilog(n, n) should be 1, not 0). Please see this PR:

theirix#1

Otherwise this looks good to me.

alamb · 2026-06-02T17:23:53Z

+        if base <= Self::ONE {
+            return None;
+        }
+        if self <= base {


Shouldn't ilog(n, n) be 1 (not zero)?

Update: yes I think it should -- here is a PR that fixes it on your forl:

arrow-buffer: i256: fix ilog off-by-one when self == base theirix/arrow-rs#1

arrow-buffer: i256: fix ilog off-by-one when self == base

theirix · 2026-06-02T18:03:49Z

Thanks @theirix and @Rafferty97 -- this is very close, but i think I found a bug (ilog(n, n) should be 1, not 0). Please see this PR:

arrow-buffer: i256: fix ilog off-by-one when self == base theirix/arrow-rs#1

Otherwise this looks good to me.

Thank you very much, @alamb, it's a good spot! Merged your fix

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alamb

Thank you @theirix and @Rafferty97

I pushed a small fmt fix to get CI to pass and I think this looks good to me now

THanks again

i256: Implement ilog via i256 crate

ca3d717

github-actions Bot added the arrow Changes to the arrow crate label Feb 20, 2026

theirix marked this pull request as ready for review February 20, 2026 22:19

Rafferty97 suggested changes Mar 12, 2026

View reviewed changes

theirix added 3 commits March 13, 2026 09:41

Refine tests

3cd316e

Use available I256::checked_ilog2

f09d53f

Remove checks enforced by i256

801a7d6

Rafferty97 approved these changes Mar 13, 2026

View reviewed changes

alamb mentioned this pull request Apr 21, 2026

Release arrow-rs / parquet Minor version 58.2.0 (April 2026) #9109

Closed

19 tasks

Native integer logs implementation

fb9d556

theirix mentioned this pull request May 31, 2026

Release arrow-rs / parquet Major version 59.0.0 (May 2026) #9110

Closed

12 tasks

alamb changed the title ~~arrow-buffer: i256: implement ilog via i256 crate~~ arrow-buffer: i256: implement ilog Jun 2, 2026

alamb and others added 2 commits June 2, 2026 13:34

alamb mentioned this pull request Jun 2, 2026

arrow-buffer: i256: fix ilog off-by-one when self == base theirix/arrow-rs#1

Merged

alamb reviewed Jun 2, 2026

View reviewed changes

Merge pull request #1 from alamb/alamb/i256-ilog-fix

eacba96

arrow-buffer: i256: fix ilog off-by-one when self == base

chore: fix rustfmt trailing whitespace

a7d21c7

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alamb approved these changes Jun 2, 2026

View reviewed changes

alamb merged commit 97f4b14 into apache:main Jun 3, 2026
26 checks passed

Uh oh!

Conversation

theirix commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 22, 2026

Uh oh!

theirix commented Apr 22, 2026

Uh oh!

alamb commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theirix commented Apr 24, 2026

Uh oh!

theirix commented May 24, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theirix commented Jun 2, 2026

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

theirix commented Feb 20, 2026 •

edited

Loading

alamb commented Apr 24, 2026 •

edited

Loading

alamb left a comment •

edited

Loading